New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove eth-account deprecation warnings #1468

Merged

kclowes merged 1 commit into ethereum:master from kclowes:change-to-from-key

Oct 17, 2019

Collaborator

kclowes commented Oct 4, 2019 •

edited

Loading

What was wrong?

There were lots of deprecation warnings from eth-account when the tests were run.

How was it fixed?

Began using the new eth-account methods that were introduced.

Todo:

Add entry to the release notes

Cute Animal Picture

kclowes force-pushed the change-to-from-key branch from 8b31cff to 5689026 Compare

October 4, 2019 17:32

Collaborator Author

kclowes commented Oct 4, 2019

@carver or @pipermerriam I am not sure why encode_defunct is working for text in this test but not for the bytes values in this test. Can you see what I'm doing wrong?

Member

pipermerriam commented Oct 4, 2019

@kclowes taking a look at the source code for those two functions it doesn't appear they can be swapped 1:1. Looks like the return value from encode_defunct is a SignableMessage type where-as the previous defunct_hash_message returns a 32-byte message hash. You'll either have to update the assertions or do some extra lifting to the get message hash.

Contributor

MatthiasLohr commented Oct 7, 2019

Having these fixed would be really nice! Can I help somehow?

Collaborator Author

kclowes commented Oct 8, 2019

@MatthiasLohr I am at Devcon and probably won't have time to get to this PR this week so feel free to pick it up. I think fixing those last few failing tests are what's left to do!

kclowes force-pushed the change-to-from-key branch from 5689026 to 4f4fb49 Compare

October 16, 2019 20:29

kclowes changed the title ~~[WIP] Remove some deprecation warnings~~ Remove eth-account deprecation warnings


          Remove eth-account deprecation warnings

4c93901

kclowes force-pushed the change-to-from-key branch from 4f4fb49 to 4c93901 Compare

October 16, 2019 21:50

kclowes requested a review from pipermerriam

October 16, 2019 22:00

pipermerriam approved these changes

View reviewed changes

pipermerriam reviewed

View reviewed changes

Member

pipermerriam left a comment

Have you verified that you've bumped the eth-account dependency sufficiently high to ensure these new APIs are present?

Collaborator Author

kclowes commented Oct 17, 2019

Good thought! Yep, eth-account is bumped to 0.4.0 which has these new APIs 👍

kclowes merged commit 724b0f5 into ethereum:master

kclowes deleted the change-to-from-key branch

October 17, 2019 20:00

carver reviewed

View reviewed changes

Collaborator

carver left a comment

MInor rename suggestion due to the change in the message-signing API.

tests/core/eth-module/test_accounts.py

    
                  message_hash = defunct_hash_message(text=message)

                  from_account = acct.recoverHash(message_hash, vrs=(v, r, s))

                  message_hash = encode_defunct(text=message)

                  from_account = acct.recover_message(message_hash, vrs=(v, r, s))

Collaborator

carver Oct 17, 2019

Since encode_defunct returns a (signable) message now, the variable name should probably be changed from message_hash. Maybe signable_message, encoded_message or encoded is a reasonable name now.

Another very reasonable option is to rename message => message_text, and message_hash => message. (this might by my favorite)

tests/core/eth-module/test_accounts.py

-                  from_account = acct.recoverHash(msg_hash, signature=signature_bytes)
-                  assert from_account == '0xFeC2079e80465cc8C687fFF9EE6386ca447aFec4'
+                  msg_hash = encode_defunct(b'\xbb\r\x8a\xba\x9f\xf7\xa1<N,s{i\x81\x86r\x83{\xba\x9f\xe2\x1d\xaa\xdd\xb3\xd6\x01\xda\x00\xb7)\xa1')  # noqa: E501
+                  from_account = acct.recover_message(msg_hash, signature=signature_bytes)

Collaborator

carver Oct 17, 2019

Same here, msg_hash is an inaccurate variable name now.

tests/core/eth-module/test_accounts.py

-                  from_account = acct.recoverHash(msg_hash, vrs=map(to_hex, (v, r, s)))
-                  assert from_account == '0xFeC2079e80465cc8C687fFF9EE6386ca447aFec4'
+                  msg_hash = encode_defunct(b'\xbb\r\x8a\xba\x9f\xf7\xa1<N,s{i\x81\x86r\x83{\xba\x9f\xe2\x1d\xaa\xdd\xb3\xd6\x01\xda\x00\xb7)\xa1')  # noqa: E501
+                  from_account = acct.recover_message(msg_hash, vrs=(v, r, s))

Collaborator

carver Oct 17, 2019

and here

tests/core/eth-module/test_accounts.py

-                  from_account = acct.recoverHash(msg_hash, vrs=(v, r, s))
-                  assert from_account == '0xFeC2079e80465cc8C687fFF9EE6386ca447aFec4'
+                  msg_hash = encode_defunct(b'\xbb\r\x8a\xba\x9f\xf7\xa1<N,s{i\x81\x86r\x83{\xba\x9f\xe2\x1d\xaa\xdd\xb3\xd6\x01\xda\x00\xb7)\xa1')  # noqa: E501
+                  from_account = acct.recover_message(msg_hash, vrs=(v, r, s))

Collaborator

carver Oct 17, 2019

ditto

tests/core/eth-module/test_accounts.py

@@ @@ -230,18 +231,22 @@ def test_eth_account_recover_vrs_standard_v(acct): @@
                   ids=['web3js_example', '31byte_r_and_s'],
               )
               def test_eth_account_sign(acct, message, key, expected_bytes, expected_hash, v, r, s, signature):
-                  message_hash = defunct_hash_message(text=message)
-                  assert message_hash == expected_hash
+                  message_hash = encode_defunct(text=message)

Collaborator

carver Oct 17, 2019

ditto

kclowes mentioned this pull request

Rename message_hash to message in account tests #1472

Merged

1 task

MatthiasLohr mentioned this pull request

fixed deprecation warning #1474

Merged

1 task

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet